-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
utils/pypi: ignore test resources when counting matches #16790
Conversation
Signed-off-by: Michael Cho <michael@michaelcho.dev>
@@ -358,7 +358,7 @@ def self.update_python_resources!(formula, version: nil, package_name: nil, extr | |||
|
|||
ohai "Updating resource blocks" unless silent | |||
Utils::Inreplace.inreplace formula.path do |s| | |||
if s.inreplace_string.scan(inreplace_regex).length > 1 | |||
if T.must(s.inreplace_string.split(/^ test do\b/, 2).first).scan(inreplace_regex).length > 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it genuinely impossible that the T.must
won't be truthy here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way I can think of is if the Formula file is entirely blank. In this case, s.inreplace_string == ""
and that will lead to nil:
❯ brew ruby -e 'p "".split(/^ test do\b/, 2).first'
nil
If the file doesn't exist, brew
fails earlier due to File.binread
. If the file has at least a single character then it will return a String:
❯ brew ruby -e 'p " ".split(/^ test do\b/, 2).first'
" "
I didn't think that would happen, but if there is an odd situation which allows for it then I can change check to something safer, e.g.
if T.must(s.inreplace_string.split(/^ test do\b/, 2).first).scan(inreplace_regex).length > 1 | |
formula_contents_without_test = s.inreplace_string.split(/^ test do\b/, 2).first | |
if formula_contents_without_test.nil? || formula_contents_without_test.scan(inreplace_regex).length > 1 |
if T.must(s.inreplace_string.split(/^ test do\b/, 2).first).scan(inreplace_regex).length > 1 | |
if s.inreplace_string.split(/^ test do\b/, 2).first&.scan(inreplace_regex)&.length.to_i > 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great explanation, thanks again @cho-m!
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Simple way to ignore resources inside test block when counting matches. We later use
s.sub!
which should only modify the first match..split
should return at least 1 string in an array as long ass.inreplace_string
is not the empty string.At least handles main issue in #16704 but we may still want to document and improve handling of test resources.